Release dependent modules only after the worker has finished for scheduled modules #39245

makortel · 2022-08-29T20:15:56Z

PR description:

In #39064 (comment) we found that the PuttableProductResolver::putProduct() releasing the task(s) for dependent modules can lead to rare problems if the producing module produces two (or more) products that refer to each other. Specifically, a consuming module may get run when only one of the products has been put in place, that can lead to deference of a Ref to fail on "missing product".

All consuming modules call the prefetchAsync_(), which for PuttableProductResolver already creates a task to release the dependent modules (m_waitingTasks), and inserts that into the Worker's WaitingTaskList (that is released after the Worker finishes). I think we could just rely on that ~~, and therefore this PR removes the override of putProduct() from PuttableProductResolver~~. This assumption turned out to be wrong, because there is also another use case for PuttableProductResolver: PuttableSourceBase sources (and TestProcessor, where it is also used like for sources).

As a simple way forward to fulfill the requirements of both use cases, this PR makes PuttableProductResolver to directly use the Worker's WaitingTaskList in prefetching, if a Worker has been associated to the PuttableProductResolver. If no Worker has been associated, the PuttableProductResolver assumes it is from a Source (or similar), and the product is available for any consuming module without any further waiting in the prefetching. See also #39245 (comment) for some further discussion.

Resolves #39064 .

PR validation:

The reproducer I mentioned in #39064 (comment) succeeds with this change. Framework unit tests pass.

cmsbuild · 2022-08-29T20:24:08Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39245/31880

This PR adds an extra 28KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/src/ProductResolvers.cc modified in PR(s): Add Transformer ability to modules #38454
- File FWCore/Framework/src/ProductResolvers.h modified in PR(s): Add Transformer ability to modules #38454

cmsbuild · 2022-08-29T20:24:33Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

FWCore/Framework (core)

@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks.
@wddgit this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

makortel · 2022-08-29T20:25:35Z

enable threading

makortel · 2022-08-29T20:25:41Z

@cmsbuild, please test

makortel · 2022-08-29T20:25:50Z

@Dr15Jones Would you agree?

Dr15Jones · 2022-08-29T21:00:10Z

I'm not sure that works for the mixing module which doesn't (I think) do any prefetching.

FWCore/Framework/src/ProductResolvers.cc

makortel · 2022-08-29T21:18:00Z

Do you mean the SecondaryEventProvider that is being used by PileUp

cmssw/Mixing/Base/src/PileUp.cc

Lines 106 to 109 in 3cac6aa

    
           if (pset.existsAs<std::vector<ParameterSet> >("producers", true)) { 
        
             std::vector<ParameterSet> producers = pset.getParameter<std::vector<ParameterSet> >("producers"); 
        
             provider_ = std::make_unique<SecondaryEventProvider>(producers, *productRegistry_, processConfiguration_); 
        
           }

that is being used by BMixingModule

cmssw/Mixing/Base/interface/BMixingModule.h

Lines 108 to 109 in 3cac6aa

    
           // input, cosmics, beamhalo_plus, beamhalo_minus 
        
           std::vector<std::shared_ptr<PileUp>> inputSources_;

?

On a first look SecondaryEventProvider sets up the workers as unscheduled

cmssw/Mixing/Base/src/SecondaryEventProvider.cc

Lines 51 to 67 in 3cac6aa

    
           SecondaryEventProvider::SecondaryEventProvider(std::vector<ParameterSet>& psets, 
        
                                                          ProductRegistry& preg, 
        
                                                          std::shared_ptr<ProcessConfiguration> processConfiguration) 
        
               : exceptionToActionTable_(new ExceptionToActionTable), 
        
                 workerManager_(std::make_shared<ActivityRegistry>(), *exceptionToActionTable_) { 
        
             std::vector<std::string> shouldBeUsedLabels; 
        
             std::set<std::string> unscheduledLabels; 
        
             const PreallocationConfiguration preallocConfig; 
        
             for (auto& pset : psets) { 
        
               std::string label = pset.getParameter<std::string>("@module_label"); 
        
               workerManager_.addToUnscheduledWorkers( 
        
                   pset, preg, &preallocConfig, processConfiguration, label, unscheduledLabels, shouldBeUsedLabels); 
        
             } 
        
             if (!unscheduledLabels.empty()) { 
        
               preg.setUnscheduledProducts(unscheduledLabels); 
        
             } 
        
           }  // SecondaryEventProvider::SecondaryEventProvider

But maybe there is some hidden detail that deserves deeper look. Unfortunately this won't be caught by any tests as the functionality has been unused for some years (and Mixing/Base has no tests).

cmsbuild · 2022-08-30T00:47:52Z

-1

Failed Tests: UnitTests RelVals RelVals-THREADING
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cb5f3c/27179/summary.html
COMMIT: edbadba
CMSSW: CMSSW_12_6_X_2022-08-29-1100/el8_amd64_gcc10
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39245/27179/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found errors in the following unit tests:

---> test TestFWCoreFrameworkTrigBit had ERRORS

RelVals

The relvals timed out after 4 hours.

RelVals-THREADING

The relvals timed out after 4 hours.

makortel · 2022-08-30T01:17:09Z

Well, something gets wrong

Begin processing the 1st record. Run 1, Event 1, LumiSection 1 on stream 0 at 29-Aug-2022 22:49:57.257 CEST


A fatal system signal has occurred: external termination request
The following is the call stack containing the origin of the signal.


---> test TestFWCoreFrameworkTrigBit had ERRORS
Mon Aug 29 23:49:32 CEST 2022

makortel · 2022-08-30T01:20:06Z

In addition, specifically HARVESTING step seems to get stuck (because of starvation of tasks in TBB)

Dr15Jones · 2022-08-30T13:31:29Z

@makortel maybe the code that transitions from one module on a path to the next module on a path is adding to the module proxy's WaitingTaskList and since it is not a 'consumes' relation your change will fail? That it, it requires the release of the WaitingTaskList on the put if no module 'consumes' that data and it only runs because it is on a Path?

cmsbuild · 2022-09-22T15:24:50Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39245/32217

This PR adds an extra 40KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/src/StreamSchedule.cc modified in PR(s): Modified data product delete early system #38983, Use "best matching" EDAlias in module dependence checks when placing ConditionalTask modules in Path #39409

cmsbuild · 2022-09-22T15:25:16Z

Pull request #39245 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again.

makortel · 2022-09-22T15:31:31Z

@cmsbuild, please test

cmsbuild · 2022-09-22T21:57:02Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-cb5f3c/27739/summary.html
COMMIT: e8a6e6a
CMSSW: CMSSW_12_6_X_2022-09-22-1100/el8_amd64_gcc10
Additional Tests: THREADING
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39245/27739/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3624368
DQMHistoTests: Total failures: 5
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3624341
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

makortel · 2022-09-23T00:58:15Z

+1

cmsbuild · 2022-09-23T00:58:37Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

makortel · 2022-09-23T01:06:36Z

type bug-fix

perrotta · 2022-09-23T07:00:48Z

+1

Fully trusting the gurus who contributed to this PR
PR tests pass successfully

cmsbuild added this to the CMSSW_12_6_X milestone Aug 29, 2022

cmsbuild added code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Aug 29, 2022

This was referenced Aug 29, 2022

Crashes in muon HLT reconstruction (reco::TrackExtra product not found) #39064

Closed

Change PutTableProductResolver to use the Worker's WaitingTaskList for the WaitingTasks of dependent modules cms-sw/framework-team#428

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 29, 2022

cmsbuild added tests-started and removed tests-pending labels Aug 29, 2022

Dr15Jones reviewed Aug 29, 2022

View reviewed changes

FWCore/Framework/src/ProductResolvers.cc Show resolved Hide resolved

cmsbuild added tests-rejected and removed tests-started labels Aug 30, 2022

cmsbuild mentioned this pull request Sep 7, 2022

Add Transformer ability to modules #38454

Merged

Initialize stream workers before processing empty (End)Paths

291b1f4

makortel force-pushed the puttableProductResolverPutProduct branch from edbadba to f88c583 Compare September 21, 2022 15:52

cmsbuild removed the tests-rejected label Sep 21, 2022

makortel force-pushed the puttableProductResolverPutProduct branch from f88c583 to e8a6e6a Compare September 22, 2022 15:17

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Sep 22, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 22, 2022

cmsbuild added tests-started and removed tests-pending labels Sep 22, 2022

cmsbuild added tests-approved and removed tests-started labels Sep 22, 2022

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Sep 23, 2022

cmsbuild added the bug-fix label Sep 23, 2022

This was referenced Sep 23, 2022

[12_4_X] Release dependent modules only after the worker has finished for scheduled modules #39484

Merged

[12_5_X] Release dependent modules only after the worker has finished for scheduled modules #39485

Merged

cmsbuild added orp-approved and removed orp-pending labels Sep 23, 2022

cmsbuild merged commit fbfd2d7 into cms-sw:master Sep 23, 2022

makortel deleted the puttableProductResolverPutProduct branch September 23, 2022 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release dependent modules only after the worker has finished for scheduled modules #39245

Release dependent modules only after the worker has finished for scheduled modules #39245

makortel commented Aug 29, 2022 •

edited

Loading

cmsbuild commented Aug 29, 2022

cmsbuild commented Aug 29, 2022

makortel commented Aug 29, 2022

makortel commented Aug 29, 2022

makortel commented Aug 29, 2022

Dr15Jones commented Aug 29, 2022

makortel commented Aug 29, 2022

cmsbuild commented Aug 30, 2022

makortel commented Aug 30, 2022

makortel commented Aug 30, 2022

Dr15Jones commented Aug 30, 2022

cmsbuild commented Sep 22, 2022

cmsbuild commented Sep 22, 2022

makortel commented Sep 22, 2022

cmsbuild commented Sep 22, 2022

makortel commented Sep 23, 2022

cmsbuild commented Sep 23, 2022

makortel commented Sep 23, 2022

perrotta commented Sep 23, 2022

Release dependent modules only after the worker has finished for scheduled modules #39245

Release dependent modules only after the worker has finished for scheduled modules #39245

Conversation

makortel commented Aug 29, 2022 • edited Loading

PR description:

PR validation:

cmsbuild commented Aug 29, 2022

cmsbuild commented Aug 29, 2022

makortel commented Aug 29, 2022

makortel commented Aug 29, 2022

makortel commented Aug 29, 2022

Dr15Jones commented Aug 29, 2022

makortel commented Aug 29, 2022

cmsbuild commented Aug 30, 2022

Unit Tests

RelVals

RelVals-THREADING

makortel commented Aug 30, 2022

makortel commented Aug 30, 2022

Dr15Jones commented Aug 30, 2022

cmsbuild commented Sep 22, 2022

cmsbuild commented Sep 22, 2022

makortel commented Sep 22, 2022

cmsbuild commented Sep 22, 2022

Comparison Summary

makortel commented Sep 23, 2022

cmsbuild commented Sep 23, 2022

makortel commented Sep 23, 2022

perrotta commented Sep 23, 2022

makortel commented Aug 29, 2022 •

edited

Loading